Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix gcc uninitialized warning in FreeBSD zio_crypt.c #16688

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Fix gcc uninitialized warning in FreeBSD zio_crypt.c #16688

merged 1 commit into from
Oct 29, 2024

Conversation

DimitryAndric
Copy link
Contributor

Fix gcc uninitialized warning in FreeBSD zio_crypt.c

With gcc we are seeing the following -Werror warning:

In file included from /workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/sunddi.h:28,
                 from /workspace/src/sys/contrib/openzfs/include/sys/zfs_context.h:69:
In function 'zfs_uio_init',
    inlined from 'zio_do_crypt_data' at /workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1690:2:
/workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/uio.h:102:45: error: 'puio_s.uio_offset' is used uninitialized [-Werror=uninitialized]
  102 |                 zfs_uio_soffset(uio) = uio_s->uio_offset;
      |                                        ~~~~~^~~~~~~~~~~~
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c: In function 'zio_do_crypt_data':
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1683:20: note: 'puio_s' declared here
 1683 |         struct uio puio_s, cuio_s;
      |                    ^~~~~~
In function 'zfs_uio_init',
    inlined from 'zio_do_crypt_data' at /workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1691:2:
/workspace/src/sys/contrib/openzfs/include/os/freebsd/spl/sys/uio.h:102:45: error: 'cuio_s.uio_offset' is used uninitialized [-Werror=uninitialized]
  102 |                 zfs_uio_soffset(uio) = uio_s->uio_offset;
      |                                        ~~~~~^~~~~~~~~~~~
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c: In function 'zio_do_crypt_data':
/workspace/src/sys/contrib/openzfs/module/os/freebsd/zfs/zio_crypt.c:1683:28: note: 'cuio_s' declared here
 1683 |         struct uio puio_s, cuio_s;
      |                            ^~~~~~

Indeed, zfs_uio_init() does:

static __inline void
zfs_uio_init(zfs_uio_t *uio, struct uio *uio_s)
{
        memset(uio, 0, sizeof (zfs_uio_t));
        if (uio_s != NULL) {
                GET_UIO_STRUCT(uio) = uio_s;
                zfs_uio_soffset(uio) = uio_s->uio_offset;
        }
}

while the code in zio_crypt.c has:

/*
 * Primary encryption / decryption entrypoint for zio data.
 */
int
zio_do_crypt_data(boolean_t encrypt, zio_crypt_key_t *key,
    dmu_object_type_t ot, boolean_t byteswap, uint8_t *salt, uint8_t *iv,
    uint8_t *mac, uint_t datalen, uint8_t *plainbuf, uint8_t *cipherbuf,
    boolean_t *no_crypt)
{
        int ret;
        boolean_t locked = B_FALSE;
        uint64_t crypt = key->zk_crypt;
        uint_t keydata_len = zio_crypt_table[crypt].ci_keylen;
        uint_t enc_len, auth_len;
        zfs_uio_t puio, cuio;
        struct uio puio_s, cuio_s;
        uint8_t enc_keydata[MASTER_KEY_MAX_LEN];
        crypto_key_t tmp_ckey, *ckey = NULL;
        freebsd_crypt_session_t *tmpl = NULL;
        uint8_t *authbuf = NULL;

        zfs_uio_init(&puio, &puio_s);
        zfs_uio_init(&cuio, &cuio_s);
        memset(GET_UIO_STRUCT(&puio), 0, sizeof (struct uio));
        memset(GET_UIO_STRUCT(&cuio), 0, sizeof (struct uio));

So between the declaration of puio_s and cuio_s, there is no initialization of these variables before zfs_uio_init() gets called.

Similar to the Linux variant of zio_crypt.c, I think it would be better to memset the structs puio_s and cuis_s before calling zfs_uio_init().

@DimitryAndric
Copy link
Contributor Author

@tsoome @amotin

@DimitryAndric DimitryAndric force-pushed the fix-gcc-uninitialized-warning-1 branch 3 times, most recently from 083583f to e906aec Compare October 25, 2024 16:31
@DimitryAndric
Fix gcc uninitialized warning in FreeBSD zio_crypt.c
853206f 
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Signed-off-by: Dimitry Andric <dimitry@andric.com>
@DimitryAndric DimitryAndric force-pushed the fix-gcc-uninitialized-warning-1 branch from e906aec to 853206f Compare October 25, 2024 16:32
amotin
amotin approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems right. Thanks.

tsoome
tsoome approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@tsoome tsoome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Oct 29, 2024
@amotin amotin merged commit 2bf1520 into openzfs:master Oct 29, 2024
20 checks passed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Nov 1, 2024
@DimitryAndric @behlendorf
Fix gcc uninitialized warning in FreeBSD zio_crypt.c
f3823a9 
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Toomas Soome <tsoome@me.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Dimitry Andric <dimitry@andric.com>
Closes openzfs#16688
ptr1337 pushed a commit to CachyOS/zfs that referenced this pull request Nov 14, 2024
@DimitryAndric @tonyhutter
Fix gcc uninitialized warning in FreeBSD zio_crypt.c
73b3e8a 
In FreeBSD's `zio_do_crypt_data()`, ensure that two `struct uio`
variables are cleared before copying data out of them. This avoids
accessing garbage data, and fixes gcc `-Wuninitialized` warnings.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Toomas Soome <tsoome@me.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Dimitry Andric <dimitry@andric.com>
Closes openzfs#16688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsoome tsoome tsoome approved these changes

@amotin amotin amotin approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DimitryAndric @behlendorf @amotin @tsoome